Repository navigation
Conversation
Nationwide reports in the app now take congressional districts from a separate run of each state and DC on that state's dataset, so a district shows the same figures in a nationwide report as in its state's report. National runs no longer need to compute them. _should_build_us_congressional_district_impact now allows only state/* regions, in any case. National runs (no region, or us) and region-group runs return congressional_district_impact null. Segmented national children run as region groups, and the segmented national reduce and Stage 12 aggregation build their output with the national region, so all of them follow the same check. District and place runs still compute none. The contract field is already nullable. Tests, seen failing before the change: - test_region_group_resolution.py: state/ca and STATE/CA are allowed; None, us and region_group/... are refused - test_simulation_output_builder.py: the output builder skips the district calculation for None, us and a region group, and still runs it for state/ut Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
National runs now return congressional_district_impact null, so the beta check in test_calculate_default_model would fail against them. A new helper, assert_no_congressional_district_results, checks that the field is present and null, and test_calculate_default_model uses it in place of the at-large district check. The state checks still expect UT- districts. _AT_LARGE_DISTRICT_IDS and assert_congressional_district_results' expected_district_ids option served only the national check, so they go. Tests, seen failing before the helper existed: local tests that the helper accepts a null field, in a dict or a generated client object, and refuses an empty district list, a listed district, a generated client object with districts, and a result without the field. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The docstring of _should_build_us_congressional_district_impact and the changelog item said Stage 12 aggregation computes no districts. Stage 12 aggregation passes on the report's own region, so it computes none for a national report, but a report for one state still computes that state's districts. Both now say so. Documentation only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
main added two changelog items at the top of the changed section, where this branch also added one, so the two sides conflicted. Both are kept, with this branch's item first. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
main replaced changelog_entry.yaml with changelog fragments (#979), so this pull request's entry moves to changelog.d/954.changed.md. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The gateway sends job-status responses with response_model_exclude_none, so once national runs return congressional_district_impact None the deployed result has no such field. assert_no_congressional_district_results first required the field to be present, so the beta check of a US national result would have failed after deploy and stopped the production deploy. It now reads the field with .get(...), as assert_no_uk_area_results does, and its local test passes results without the field; that test failed before the helper changed. The test that refused a result without the field is removed. The changelog fragment now says the field is null in the output and left out of the job-status response, and the district check's docstring says Stage 12 aggregation matches only a state/... spelling, because it passes the region as the request sent it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Removing the national check also removed the only deployed check of the seven at-large states, whose districts nationwide reports now take from each state's own run. The two Utah beta tests sent the same request, so the pinned-version one now runs state/vt and expects exactly VT-01, through a restored expected_district_ids option that matches a state's district IDs exactly. The option's local tests failed first, because the option did not exist. The beta suite keeps its number of tests, so BETA_PYTEST_WORKERS is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
expected_district_ids compared sets, so a state result listing VT-01 twice passed. It now compares the sorted lists; the new repeated-district case failed first. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #954
Same-Repository Draft PR Check
PolicyEngine/policyengine-sim-api, not a personal fork.Do not merge yet
Merging to
maindeploys staging and then production. Until PolicyEngine/policyengine-app-v2#1439 is released, the app reads congressional districts from nationwide results, so this may merge only after that release has been in production for at least a day.That app release in turn waits for the Public API's PolicyEngine/policyengine-api#4064 and this repository's #940 to deploy.
Summary
Why:
What changes:
_should_build_us_congressional_district_impact(simulation_output_geographic.py) now returns true only for astate/*region, in any case.congressional_district_impactis null in the output, and the gateway's job-status response, sent withresponse_model_exclude_none=True, leaves the field out):us;state/...spelling matches there (a bareutcomputes none on that path, as before this change; the Public API always sendsstate/...).simulation_output_geographic.pyandsegmented_national_reduce.py, and a changelog entry.assert_no_congressional_district_results, checks that the field is absent or null;test_calculate_default_model, a US national run, uses it in place of the at-large district check;UT-districts, and the pinned-version check now runs Vermont and expects exactlyVT-01._AT_LARGE_DISTRICT_IDSserved only the national check, so it is removed.assert_congressional_district_results'expected_district_idsoption, which also served only that check, now matches a state's district IDs exactly (item 5 below).Older model versions: a deployment rebuilds only the Modal app for the current policyengine.py version. Apps already deployed for older versions are not rebuilt and keep their code, so a request pinned to an older model version can still return nationwide districts.
Testing
Each commit is red-green: the tests were written first and run before the code change.
TestCongressionalDistrictRegions::test__rejects_national_and_group(None,us,region_group/…);test_builder_skips_districts_for_national_and_region_group_runsforNone,usand a region group, because the output builder still ran the district calculation.test__allows_states(state/ca,STATE/CA);test__rejects_district_and_place;test_builder_builds_districts_for_state_runs(state/ut).test_national_us_assertions_accept_a_result_without_districtsnow also passes a dict and a generated client object without the field; it failed with "Missing 'congressional_district_impact'" before the helper was changed to.get(...) is None, asassert_no_uk_area_resultsalready does. The test that refused a result without the field is removed.test_calculate_specific_modelnow runsstate/vtand expects exactlyVT-01, throughassert_congressional_district_results'expected_district_idsoption, whichmainhas as a subset check for the national result: an earlier commit here removed it with that check, and this one restores it as an exact match of the sorted IDs, so a district listed twice fails too (review round 3). Its local tests (test_state_assertions_accept_an_at_large_state_with_its_one_district,test_state_assertions_reject_other_district_ids) failed first, because the option did not exist on this branch and then because a repeated district passed. The beta suite keeps the same number of tests.Run locally:
uv run --frozen pytest tests/inprojects/policyengine-simulation-executor: 1881 passed, 22 skipped.uv run --frozen pytest tests/simulation/test_calculate.py -m "not beta_only"inprojects/policyengine-apis-integ: 23 passed (before the review rounds, which removed one of these tests and added two).make check: Ruff format and lint, and Pyright for every package, passed.GITHUB_BASE_REF=main python scripts/run_quality_guards.py: every guard passed.BASE_SHA=origin/main .github/scripts/check-changelog-fragment.sh.Not run locally: the
beta_onlyintegration tests, which run against the beta deployment. The national check runs on the app that beta'slatestroute points to, andlatestmoves only to a newer policyengine.py version (or when forced): if a branch with a newer version and without this change were deployed to beta by hand, that check would fail, and stop the production deploy, untillatestpoints to an app with this change. After this deploys to beta, a US national result from the current version's app should have nocongressional_district_impactfield, and astate/utresult should still listUT-districts. The null check applies only to the current version's app, for the reason under "Older model versions".🤖 Generated with Claude Code